-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WebExtension tweaks for Safari and Manifest V3 #17532
Conversation
e340e37
to
39bfa2b
Compare
@queengooborg let me know if this is okay, or if there are somethings I should do differently. |
Throughout MDN, the terms "required" and "optional" are used more frequently than "mandatory" -- could we keep using the term "required" and replace "not required"/"not mandatory" with optional? |
39bfa2b
to
b99e5e0
Compare
}, | ||
{ | ||
"version_added": "42", | ||
"alternative_name": "applications", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This alternative_name
was a dummy entry to allow per-version notes here. I removed it to avoid confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this! While we're here, let's also tweak some of the wording to follow the typical formula for notes:
b99e5e0
to
88853fe
Compare
* Features that are dropped in V3 noted as "Manifest V2 only". * Features that are added in V3 noted as "Manifest V3 or later" to avoid prematurely limiting to V3 when future versions are supported. * Features that are dropped in V3 by some browsers but are supported in full in others still noted as "Manifest V2 or later".
88853fe
to
8f0f5d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this is LGTM!
Rebased and ready to merge when you can, thanks @queengooborg! |
By the way, no need to rebase and force push for BCD PRs -- we squash merge all pull requests to this repository! |
], | ||
"firefox": { | ||
"version_added": "42", | ||
"notes": "Before Firefox 48, this property was required." | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why has this been changed? This breaks some of our tools and it took me a while to figure that out since it is not mentioned anywhere in the changelog. This patch is for Safari, it would have been good to not change Firefox data or to move these changes to a separate issue/PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is actually for Manifest V3 entirely as well, and if you look at the diff, you'll notice that this is simply consolidating the statements together to adhere to the typical structure of BCD (where a behavioral difference that only applies to one browser should be a single note, rather than two separated statements).
We apologize in advance for the changes causing an issue with your tooling; however, please do note that features may be added, removed or modified at any time to match developer interest, BCD data guidelines, or for better accuracy, as explained in the Semantic versioning policy. In other words, changes such as this are expected to happen in any release and will not result in any changelog notes.
runtime.getFrameId
and drop STP reference.